-
Notifications
You must be signed in to change notification settings - Fork 326
Add support for JUL auto ECS-reformatting #2591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
run integration tests |
1 similar comment
|
run integration tests |
|
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few questions/comments.
|
|
||
| @Override | ||
| public ElementMatcher.Junction<ClassLoader> getClassLoaderMatcher() { | ||
| // todo: change when adding support for instrumentation of Tomcat and JBoss logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here for tomcat & jboss logging you mean the logging on those application servers that rely on JUL ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they rely on it as a framework, but provide their own implementations, for Loggers but also for the actual Handlers, for example - org.apache.juli.FileHandler
...n/src/main/java/co/elastic/apm/agent/jul/reformatting/JulLogReformattingInstrumentation.java
Show resolved
Hide resolved
...n/src/main/java/co/elastic/apm/agent/jul/reformatting/JulLogReformattingInstrumentation.java
Show resolved
Hide resolved
| return isBootstrapClassLoader(); | ||
| } | ||
|
|
||
| public static class FileReformattingInstrumentation extends JulLogReformattingInstrumentation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] because the publish method is defined on Handler class, maybe we could have a common method matcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't because it will add yet another level to the inheritance hierarchy, making it three-level hierarchy for three instrumentations. I figured it's too much
| private static final ThreadLocal<String> currentPattern = new ThreadLocal<>(); | ||
| private static final ThreadLocal<Path> currentExampleLogFile = new ThreadLocal<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] is there any reason not to rely on co.elastic.apm.agent.sdk.weakconcurrent.WeakConcurrent#buildThreadLocal here ? Also, it might be worth checking if we need to have those fields within an @GlobalState.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
co.elastic.apm.agent.sdk.weakconcurrent.WeakConcurrent#buildThreadLocal here ?
There is no need because the value in both cases is basic Java and the this instrumentation is using the bootstrap CL. It is in my todo list to add an asciidoc regarding ThreadLocal and when to use which, it just doesn't get to the top of priorities still...
Also, it might be worth checking if we need to have those fields within an @globalstate
This plugin instruments basic Java types loaded by the bootstrap CL. Why should we require it to be a @GlobalState?
| public boolean onAppendEnter(FileHandler fileHandler, String pattern, File exampleLogFile) { | ||
| try { | ||
| currentPattern.set(pattern); | ||
| currentExampleLogFile.set(exampleLogFile.toPath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] here we get the "first file" from the implementation internal state, how do we know that it's representative of all the files that the FileHandler might be writing to ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at java.util.logging.FileHandler#openFiles, you'll see that all files are created based on a single pattern. We only use this for the enclosing log directory. If the user makes the pattern so that directories get changed between logs, that is unexpected and we will just use the first dir. I don't think we need to add special handling for such case, but we can improve based on feedback.
What does this PR do?
Closes #2490
Todo
Checklist
SHADEandOVERRIDEoptions manually with Java 7 and Java 17 test apps